-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(max): deep research template system #39104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
feat: deep research templates chore: mypy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
ee/hogai/graph/deep_research/onboarding/nodes.py, line 77 (link)logic: Resetting current_run_notebooks to empty list may conflict with the skip_onboarding flow that expects notebooks to persist
10 files reviewed, 5 comments
| assistant_initial_state = None | ||
| if initial_state is not None: | ||
| if isinstance(initial_state, (AssistantState | PartialAssistantState)): | ||
| assistant_initial_state = initial_state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This code duplicates the type checking logic from the ASSISTANT mode. Consider extracting this pattern into a helper method to avoid repetition.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ee/hogai/assistant/assistant.py
Line: 55:58
Comment:
**style:** This code duplicates the type checking logic from the ASSISTANT mode. Consider extracting this pattern into a helper method to avoid repetition.
How can I resolve this? If you propose a fix, please make it concise.| class DeepResearchOnboardingNode(DeepResearchNode): | ||
| @property | ||
| def node_name(self) -> MaxNodeName: | ||
| return DeepResearchNodeName.NOTEBOOK_PLANNING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The node_name returns NOTEBOOK_PLANNING but the class is DeepResearchOnboardingNode - this seems inconsistent and could be confusing
Prompt To Fix With AI
This is a comment left during a code review.
Path: ee/hogai/graph/deep_research/onboarding/nodes.py
Line: 20:20
Comment:
**logic:** The node_name returns NOTEBOOK_PLANNING but the class is DeepResearchOnboardingNode - this seems inconsistent and could be confusing
How can I resolve this? If you propose a fix, please make it concise.| if not isinstance(last_message, HumanMessage): | ||
| raise ValueError("Last message is not a human message.") | ||
|
|
||
| human_content = last_message.content if last_message else "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Redundant condition - last_message is guaranteed to exist due to the check on line 42
| human_content = last_message.content if last_message else "" | |
| human_content = last_message.content |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ee/hogai/graph/deep_research/notebook/nodes.py
Line: 45:45
Comment:
**logic:** Redundant condition - `last_message` is guaranteed to exist due to the check on line 42
```suggestion
human_content = last_message.content
```
How can I resolve this? If you propose a fix, please make it concise.| if not ( | ||
| state.template_notebook_short_id and not state.template_markdown and not state.has_emitted_template_loaded | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Complex nested boolean logic makes this condition hard to understand. Consider extracting to a helper method with descriptive name like should_retrieve_template_from_db.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ee/hogai/graph/deep_research/notebook/nodes.py
Line: 107:109
Comment:
**style:** Complex nested boolean logic makes this condition hard to understand. Consider extracting to a helper method with descriptive name like `should_retrieve_template_from_db`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recap:
- I think notebooks should be sent as UI context, not as a custom field, and only as an ID, not the whole notebook, this would allow us to reuse this logic to support them in base Max; if we move to the notebook template model, it can still be just an ID as UI context maybe with a
template: Truefield, so we trigger the template flow rather than passing it as context, just an idea - Template management/streaming should be in a separate node, and I don't think the frontend needs to send a tag to say "create a custom template", it's the backend that should do that when no template is provided
- There's some unused state variables and I couldn't really understand the entire flow, so some more comments would have helped
|
|
||
| # Curated deep-research notebook templates stored as ProseMirror JSON | ||
|
|
||
| CURATED_DEEP_RESEARCH_NOTEBOOKS: list[dict] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for now, we can then move these to a model, as discussed.
|
|
||
| for key in filters: | ||
| value = filters.get(key, None) | ||
| if key == "tags" and isinstance(value, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal_tags to align with the other PR.
| # and stream it immediately by setting _latest_message. | ||
| message_for_state = self._latest_message | ||
| if not self._latest_message and self._deep_research_template: | ||
| from uuid import uuid4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import at the top
| if isinstance(self._deep_research_template, dict): | ||
| title = self._deep_research_template.get("notebook_title") | ||
|
|
||
| content = f"Load template: {title}" if title else "Load template" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this message?
| messages=[message_for_state] if message_for_state else [], | ||
| start_id=message_for_state.id if message_for_state else None, | ||
| graph_status=None, | ||
| notebook_short_id=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notebook_short_id should be removed, it's not on DeepResearchState anymore, we have add the other two notebook fields as None
| if content: | ||
| try: | ||
| nb_json = ProsemirrorJSONContent.model_validate(notebook.content) | ||
| from ee.hogai.notebook.notebook_serializer import NotebookSerializer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import at the top if possible
|
|
||
| async def _retrieve_template_markdown(self, state: DeepResearchState) -> str | None: | ||
| if not ( | ||
| state.template_notebook_short_id and not state.template_markdown and not state.has_emitted_template_loaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not state.template_markdown -> return state.template_markdown?
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also state.markdown is never set anywhere? I really don't understand the logic.
|
|
||
| def should_run_onboarding_at_start(self, state: DeepResearchState) -> Literal["onboarding", "planning", "continue"]: | ||
| # Skipping onboarding when provided a template | ||
| if state.skip_onboarding: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need skip_onboarding when we also have template_id in the state?
| human_content = last_message.content if last_message else "" | ||
| # If a template was provided, emit a synthetic "loaded notebook" message once | ||
| pre_messages: list = [] | ||
| if template_markdown and not state.has_emitted_template_loaded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is all of this needed to create custom templates?
It seems extremely convoluted. This is the flow I would expect:
- If no template is selected, the node streams the default custom template, or better, the agent writes a new draft template notebook to fill up
- If template is selected, the node simply clones and streams the templated notebook to fill up
In both cases it seems like you would need an additional node that only does this, and this node should then be relegated to just the planning.
This would also help us with separation of concerns when we want to move to the Google DR flow.
| trace_id = serializers.UUIDField(required=True) | ||
| session_id = serializers.CharField(required=False) | ||
| deep_research_mode = serializers.BooleanField(required=False, default=False) | ||
| deep_research_template = serializers.JSONField(required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're sending the whole notebook here, it's overkill
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Problem
For Max Deep Research, one possible pain point during the onboarding phase is for the user to go through receiving a long list of questions from Max that needs to be answered. This can feel tiresome and make the experience not particularly great.
Idea: Have a list of curated + custom templates (notebooks) that the user can compile in their own time and then load as onboarding step when starting Deep Research.
This PR builds on top of the foundational piece for notebooks tags (#39101) and add backend support for Max and Notebooks API.
How did you test this code?
deep_research_templates_e2e.mp4